feat: implemented fallback mechanism for when CNI does not provide an…#4439
Open
estebancams wants to merge 3 commits into
Open
feat: implemented fallback mechanism for when CNI does not provide an…#4439estebancams wants to merge 3 commits into
estebancams wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a Windows fallback mechanism for HCN network adapter selection when an adapter name policy can’t be applied (e.g., vSwitch/vEthernet scenarios), using the NC PrimaryInterfaceIdentifier to build a provider-address policy.
Changes:
- Rename the existing adapter-name policy helper and update call sites.
- Introduce a new HCN network policy helper for
ProviderAddressand apply it as a fallback in Windows HCN network configuration. - Plumb
PrimaryInterfaceIdentifierfrom the CNS NC response intonetwork.EndpointInfoand add Windows unit tests for the fallback behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| network/policy/policy_windows.go | Renames the adapter name policy helper and adds a provider-address policy helper. |
| network/network_windows.go | Applies adapter-name policy when possible; otherwise attempts provider-address fallback based on PrimaryInterfaceIdentifier. |
| network/network_windows_test.go | Adds tests validating provider-address fallback vs adapter-name behavior. |
| network/endpoint.go | Extends EndpointInfo with PrimaryInterfaceIdentifier. |
| cns/hnsclient/hnsclient_windows.go | Updates call site to renamed adapter-name policy helper. |
| cni/network/network.go | Copies PrimaryInterfaceIdentifier from NC response into EndpointInfo. |
Comment on lines
265
to
287
| if !netAdapterNamePolicyAdded { | ||
| primaryInterfaceIdentifier := nwInfo.PrimaryInterfaceIdentifier | ||
| if primaryInterfaceIdentifier == "" { | ||
| return nil, errors.New("PrimaryInterfaceIdentifier is empty. Adapter Address policy can't be applied") | ||
| } | ||
|
|
||
| var providerAddress string | ||
| // Based on cns/NetworkContainerContract.go, PrimaryInterfaceIdentifier can be either an IP or a CIDR | ||
| if ip, _, err := net.ParseCIDR(primaryInterfaceIdentifier); err == nil { | ||
| providerAddress = ip.String() | ||
| } else if ip := net.ParseIP(primaryInterfaceIdentifier); ip != nil { | ||
| providerAddress = ip.String() | ||
| } else { | ||
| return nil, fmt.Errorf("PrimaryInterfaceIdentifier %q is not a valid IP or CIDR", primaryInterfaceIdentifier) | ||
| } | ||
|
|
||
| adapterAddressPolicy, err := policy.GetHcnNetAdapterAddressPolicy(providerAddress) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Failed to serialize network adapter address policy: %w", err) | ||
| } | ||
|
|
||
| hcnNetwork.Policies = append(hcnNetwork.Policies, adapterAddressPolicy) | ||
| } |
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
… AdapterName through policy to HNS during network creation
estebancams
force-pushed
the
estebanca/cni-hns-provider-address
branch
from
a08546f to
8a491e8
Compare
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP
HNS team has made changes that allow to create a dual-stack network when an ipv4-only network has already been associated with an interface. Previously this was not possible (although it was possible to create such networks in the reverse order).
This change has a side effect in HNS behavior for when the Adapter Name is not passed as part of the creation payload in a node with multiple adapters:
Before the change:
After the change:
Coincidentally, CNI is passing an empty adapter name when there is any network already created in the node. This is because HNS fails to associate a new network with a virtual (vEthernet) adapter. This happens because the original Ethernet adapter becomes the uplink of the vSwitch where the network is created on and a new network has to be associated with that adapter.
HNS team proposed extending HNS creation API so CNI can pass instead of the adapter name, the IP of the original adapter.
The change in this PR uses that new API when adapter name is going to be passed empty to HNS during network creation. Note the change does not migrate from adapter name to IP APIs, but it only covers the existent gap. Also, this change will continue to be a best effort action and if the IP address cannot be passed to HNS for whatever reason, the call to HNS will still be made regardless (CNI should not fail in scenarios where it previously didn't)